Allow TUV-x radiator data to be updated through the API #182

boulderdaze · 2024-07-26T23:56:34Z

Closes #122

Discussion:
I ended up choosing class to create Radiator and RadiatorMap because it has private member data and the usage of friend declaration. struct is used to create Grid, GridMap, Profile and ProfileMap (they also have private member data). Although struct and class in C++ has no difference except for default access (public/private), struct objects are somewhat expected to be all public, so I might vote for replacing struct with class. What do you all think? I think having consistency is important. (all of them should be either struct or class)

class Radiator
{
  public:
   Radiator(const char *radiator_name, Grid *height_grid, Grid *wavelength_grid, Error *error);

   ...

  private:
   void *radiator_;
   void *updater_;

  friend class RadiatorMap;
}

  struct Grid
  {
    Grid(const char *grid_name, const char *units, std::size_t num_sections, Error *error);

   ...

   private:
    void *grid_;
    void *updater_;

   friend class GridMap;
   friend class Profile;
   friend class Radiator;  
 }

boulderdaze · 2024-08-06T16:52:11Z

src/test/unit/tuvx/tuvx_c_api.cpp

+  std::size_t num_vertical_layers = 3;
+  std::size_t num_wavelength_bins = 2;
+  // Allocate array as 1D
+  double* optical_depths_1D = new double[num_wavelength_bins * num_vertical_layers];
+  // Allocate an array of pointers to each row
+  double** optical_depths = new double*[num_vertical_layers];
+  // Fill in the pointers to the rows
+  for (int row = 0; row < num_vertical_layers; row++)
+  {
+    optical_depths[row] = &optical_depths_1D[row * num_wavelength_bins];
+  }
+  int i = 1;
+  for (int row = 0; row < num_vertical_layers; row++)
+  {
+    for (int col = 0; col < num_wavelength_bins; col++)
+    {
+      optical_depths[row][col] = 10 * i;
+      i++;
+    }
+  }


Do we like this? I initially wanted to use vector but couldn't find a way to make it work for the fortran APIs to be able to index the array components. (e,g., optical_depths[col][row]) @mattldawson, @K20shores, any thoughts on this implementation?

I like this solution! This seems like it will work well with the fortran wrapper, and it keeps the data contiguous in memory

boulderdaze · 2024-08-06T17:04:51Z

src/test/unit/tuvx/tuvx_c_api.cpp

+  GetRadiatorOpticalDepths(radiator, optical_depths[0], num_vertical_layers, num_wavelength_bins, &error);
+  ASSERT_TRUE(IsSuccess(error));
+  ASSERT_EQ(optical_depths[0][0], 10.0);
+  ASSERT_EQ(optical_depths[0][1], 20.0);
+  ASSERT_EQ(optical_depths[1][0], 30.0);
+  ASSERT_EQ(optical_depths[1][1], 40.0);
+  ASSERT_EQ(optical_depths[2][0], 50.0);
+  ASSERT_EQ(optical_depths[2][1], 60.0);
+


I'm not confident whether this is the correct approach. Since the indices are transposed between Fortran and C, this setup of memory allocation is in favor of fortran, so in fortran side we can do indexing as following. @mattldawson, @K20shores, could you review this?

optical_depths(:,1) = (/ 30.0, 20.0, 10.0 /) optical_depths(:,2) = (/ 70.0, 80.0, 90.0 /) call radiator%get_optical_depths( temp_od, error ) ASSERT_EQ( temp_od(1,1), 30.0 ) ASSERT_EQ( temp_od(2,1), 20.0 ) ASSERT_EQ( temp_od(3,1), 10.0 ) ASSERT_EQ( temp_od(1,2), 70.0 ) ASSERT_EQ( temp_od(2,2), 80.0 ) ASSERT_EQ( temp_od(3,2), 90.0 )

yes, I think this is the best solution. The indexing is different, but the memory should be laid out in the optimal configuration regardless of how elements are accessed, and looping in C and Fortran should just be set up to access the elements in the optimal order:

for(int i=0; i < n_i; ++i) for(int j=0; j < n_j; ++j) optical_depths[i][j] = ...

and:

do i = 1, n_i do j = 1, n_j optical_depths(j,i) = ... end do end do

That makes sense, thank you

mattldawson

looks great! Just a couple very minor suggestions. I agree with your suggestion of switching the struct to class

mattldawson · 2024-08-07T15:42:01Z

docker/Dockerfile

@@ -9,7 +9,7 @@ RUN dnf -y update \
        gfortran \
        gdb \
        git \
-        lapack-devel \
+        # lapack-devel \


we can probably just delete this line

mattldawson · 2024-08-07T15:42:15Z

docker/Dockerfile.fortran-gcc

@@ -13,7 +13,7 @@ RUN dnf -y update \
    git \
    hdf5-devel \
    json-devel \
-    lapack-devel \
+    # lapack-devel \


can just delete this

mattldawson · 2024-08-07T15:42:27Z

docker/Dockerfile.fortran-gcc.integration

@@ -14,7 +14,7 @@ RUN dnf -y update \
    git \
    hdf5-devel \
    json-devel \
-    lapack-devel \
+    # lapack-devel \


mattldawson · 2024-08-07T15:42:35Z

docker/Dockerfile.memcheck

@@ -9,7 +9,7 @@ RUN dnf -y update \
        gfortran \
        gdb \
        git \
-        lapack-devel \
+        # lapack-devel \


mattldawson · 2024-08-07T15:42:42Z

docker/Dockerfile.mpi

@@ -16,7 +16,7 @@ RUN sudo dnf -y install \
        gcc-c++ \
        gfortran \
        git \
-        lapack-devel \
+        # lapack-devel \


mattldawson · 2024-08-07T15:42:49Z

docker/Dockerfile.mpi_openmp

@@ -16,7 +16,7 @@ RUN sudo dnf -y install \
        gcc-c++ \
        gfortran \
        git \
-        lapack-devel \
+        # lapack-devel \


mattldawson · 2024-08-07T15:42:55Z

docker/Dockerfile.openmp

@@ -16,7 +16,7 @@ RUN sudo dnf -y install \
        gcc-c++ \
        gfortran \
        git \
-        lapack-devel \
+        # lapack-devel \


mattldawson · 2024-08-07T15:43:02Z

docker/Dockerfile.python

@@ -9,7 +9,7 @@ RUN dnf -y update \
        gcc-fortran \
        gdb \
        git \
-        lapack-devel \
+        # lapack-devel \


boulderdaze added 30 commits July 19, 2024 10:55

add radiator wrapper

e6b2c2a

working-on file of radiator header

63a3fa2

fix typos, and incomplete parts

42e2817

code clean up

0f4d39a

add radiator src

6cdef23

fix bugs

2267552

fix bugs

b85b1b0

fix bugs in c sources

c248e1e

fix bugs in f90 sources

90bd69a

fix bugs

21ac00e

temp add radiator test

f5256ad

fix bugs

5ac631d

passed test

0ae60e1

incomplete test

80349d4

temp file for transfer

9cc7ad3

working version

7b8714b

fix test

cab7fb0

fix bugs

29e42f6

fix fortran bugs

a6f302d

add radiator fortran test

18d47ca

test tag

0101fe3

add fortran test

f82d417

add tests back:

07cde7d

Merge branch 'main' into 122-make-radiator-updatable

0f4294d

fix docker integration

72362cc

revert cmake

ceb64d5

clean up

b75e012

code cleanup

3611be0

code clean up

24e863c

revert back to main branch

b30c733

boulderdaze marked this pull request as ready for review August 5, 2024 23:47

boulderdaze requested review from mattldawson and K20shores August 5, 2024 23:48

K20shores approved these changes Aug 6, 2024

View reviewed changes

boulderdaze commented Aug 6, 2024

View reviewed changes

mattldawson approved these changes Aug 7, 2024

View reviewed changes

boulderdaze added 2 commits August 7, 2024 11:16

Address review comments

d41d152

fix tuvx git tag

2a419e9

boulderdaze merged commit 42f5079 into main Aug 7, 2024
61 checks passed

boulderdaze deleted the 122-make-radiator-updatable branch August 7, 2024 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow TUV-x radiator data to be updated through the API #182

Allow TUV-x radiator data to be updated through the API #182

boulderdaze commented Jul 26, 2024 •

edited

Loading

boulderdaze Aug 6, 2024 •

edited

Loading

mattldawson Aug 7, 2024

boulderdaze Aug 6, 2024

mattldawson Aug 7, 2024

boulderdaze Aug 7, 2024

mattldawson left a comment

mattldawson Aug 7, 2024

boulderdaze Aug 7, 2024

mattldawson Aug 7, 2024

boulderdaze Aug 7, 2024

mattldawson Aug 7, 2024

boulderdaze Aug 7, 2024

mattldawson Aug 7, 2024

boulderdaze Aug 7, 2024

mattldawson Aug 7, 2024

boulderdaze Aug 7, 2024

mattldawson Aug 7, 2024

boulderdaze Aug 7, 2024

mattldawson Aug 7, 2024

boulderdaze Aug 7, 2024

mattldawson Aug 7, 2024

boulderdaze Aug 7, 2024

Allow TUV-x radiator data to be updated through the API #182

Allow TUV-x radiator data to be updated through the API #182

Conversation

boulderdaze commented Jul 26, 2024 • edited Loading

boulderdaze Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattldawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boulderdaze commented Jul 26, 2024 •

edited

Loading

boulderdaze Aug 6, 2024 •

edited

Loading